Skip to content

fix: sync cache only on compare returns true #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Jan 15, 2021

fixes #906

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 15, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 062de50:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration
swr using compare Issue #906

Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels good to me, but this change assumes that stateRef.current.data equals to the current cache. I'm not sure if there's any case that it will break.

@pablen
Copy link

pablen commented Feb 4, 2021

Hi team, thanks for this quick PR :)
Is there anything that can be done to get it merged?

@huozhi
Copy link
Member Author

huozhi commented Feb 5, 2021

rebased to resolve the conflicts

@huozhi
Copy link
Member Author

huozhi commented Feb 5, 2021

Feels good to me, but this change assumes that stateRef.current.data equals to the current cache. I'm not sure if there's any case that it will break.

I'd expect it won't be a problem with dequal since it supports various primtive data types. I feel it's ok to ship in a patch version and we can wait if there's any issue reports form community?

Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna ship this once all tests have passed. Thank you!

@shuding shuding merged commit 21fe398 into vercel:master Mar 5, 2021
@pablen
Copy link

pablen commented Mar 5, 2021

Thank you all!

This was referenced Mar 15, 2021
@promer94
Copy link
Collaborator

#1056
The stateRef is not sync with cache when key changes in suspense mode. if two different keys happen to have the same response data. The cache of the second key would not be updated and it will cause infinite loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compare function returning true still updates cache
4 participants